Handle invalid Cargo manifest decoding errors#179
Conversation
Catch TOML decode failures in the manifest reader script and cover it\nwith unit tests that exercise the CLI and supported fields.
Reviewer's GuideThe PR refactors the manifest reader to include consistent docstrings, type hints, and a shared description constant, and extends its error handling to catch tomllib.TOMLDecodeError. It pulls the inline release-asset upload bash logic out of the workflows into a standalone Python script and a composite GitHub Action, updates the release workflows to invoke the new action, and adds end-to-end unit tests for both scripts. A lint configuration tweak (task-tags) rounds out the CI updates. Sequence diagram for manifest reader CLI error handlingsequenceDiagram
participant User as actor User
participant CLI as Manifest Reader CLI
participant File as Cargo.toml File
participant TOML as tomllib
User->>CLI: Run CLI with field argument
CLI->>File: Open manifest file
alt File not found
CLI->>User: Print FileNotFoundError to stderr
CLI->>User: Exit with code 1
else File found
CLI->>TOML: Parse manifest file
alt Invalid TOML syntax
TOML->>CLI: Raise TOMLDecodeError
CLI->>User: Print TOMLDecodeError to stderr
CLI->>User: Exit with code 1
else Valid TOML
CLI->>CLI: Extract requested field
alt Field missing
CLI->>User: Print KeyError to stderr
CLI->>User: Exit with code 1
else Field present
CLI->>User: Print field value to stdout
CLI->>User: Exit with code 0
end
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughSummarise manifest-reading robustness and add a reusable action plus script to discover, validate and optionally upload release artefacts (dry‑run supported). Replace inline release workflow shell steps with the new action. Add tests for manifest reader and asset discovery/upload logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as read_manifest.py
participant Env as Environment
participant FS as Filesystem
participant TOML as tomllib
User->>CLI: run CLI (--field, optional --manifest)
CLI->>Env: read CARGO_TOML_PATH
alt explicit --manifest provided
CLI-->>CLI: use provided path
else env provides path
Env-->>CLI: supply path
else
CLI-->>CLI: default Cargo.toml
end
CLI->>FS: open manifest
FS-->>CLI: contents / FileNotFoundError
alt file missing
CLI-->>User: print error, exit 1
else
CLI->>TOML: parse contents
alt TOMLDecodeError
TOML-->>CLI: error
CLI-->>User: print error, exit 1
else parsed
CLI-->>CLI: get_field
alt missing/blank field
CLI-->>User: print error, exit 1
else found
CLI-->>User: print value, exit 0
end
end
end
sequenceDiagram
autonumber
actor Workflow as GitHub Workflow
participant Action as upload-release-assets Action
participant Script as scripts/upload_release_assets.py
participant FS as Filesystem
participant GH as gh CLI
Workflow->>Action: invoke with inputs (release-tag, bin-name, dist-dir, dry-run)
Action->>Script: set env and exec script
Script->>FS: list files in dist-dir
FS-->>Script: file list
Script->>Script: filter by bin_name and patterns
Script->>Script: validate non-zero size and detect duplicate names
alt validation fails
Script-->>Action: exit non-zero, print error
else validation OK
alt dry-run true
Script-->>Action: print planned uploads summary
else
loop per asset
Script->>GH: gh release upload --clobber asset --title name
GH-->>Script: success/fail
end
Script-->>Action: exit 0
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Blocking issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests_python/test_read_manifest.py:109` </location>
<code_context>
+ self.assertIn("does not exist", result.stderr)
+ self.assertEqual(result.stdout, "")
+
+ def test_main_reports_invalid_toml(self) -> None:
+ manifest = self._write_manifest("not = [valid")
+ result = subprocess.run(
+ [sys.executable, str(SCRIPT_PATH), "name", "--manifest-path", str(manifest)],
+ check=False,
+ capture_output=True,
+ text=True,
+ )
+ self.assertNotEqual(result.returncode, 0)
+ self.assertTrue(result.stderr)
+ self.assertEqual(result.stdout, "")
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for TOML files that are valid but contain unexpected structure.
Adding such a test will verify that the script handles valid TOML files missing required sections or fields appropriately, ensuring robust error handling.
```suggestion
self.assertEqual(result.stdout, "")
def test_main_reports_valid_toml_with_unexpected_structure(self) -> None:
# Write a valid TOML file that is missing required fields/sections
manifest = self._write_manifest(
"""
[unexpected_section]
foo = "bar"
"""
)
result = subprocess.run(
[sys.executable, str(SCRIPT_PATH), "name", "--manifest-path", str(manifest)],
check=False,
capture_output=True,
text=True,
)
self.assertNotEqual(result.returncode, 0)
self.assertIn("missing", result.stderr.lower())
self.assertEqual(result.stdout, "")
```
</issue_to_address>
### Comment 2
<location> `tests_python/test_read_manifest.py:59` </location>
<code_context>
+ manifest = {"package": {"name": "netsuke", "version": "1.2.3"}}
+ self.assertEqual(self.module.get_field(manifest, "version"), "1.2.3")
+
+ def test_get_field_raises_when_missing(self) -> None:
+ manifest = {"package": {"name": "netsuke"}}
+ with self.assertRaises(KeyError):
+ self.module.get_field(manifest, "version")
+
+ def test_main_reads_manifest_path_argument(self) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Consider testing for non-string field values in the manifest.
Add a test where the field exists but is a non-string type (e.g., integer, list, dict) to verify correct handling or error raising.
```suggestion
def test_get_field_returns_non_string_value(self) -> None:
manifest = {"package": {"name": "netsuke", "version": 123, "authors": ["alice", "bob"], "metadata": {"license": "MIT"}}}
# Test integer value
self.assertEqual(self.module.get_field(manifest, "version"), 123)
# Test list value
self.assertEqual(self.module.get_field(manifest, "authors"), ["alice", "bob"])
# Test dict value
self.assertEqual(self.module.get_field(manifest, "metadata"), {"license": "MIT"})
def test_main_reads_manifest_path_argument(self) -> None:
```
</issue_to_address>
### Comment 3
<location> `tests_python/test_read_manifest.py:67-72` </location>
<code_context>
result = subprocess.run(
[sys.executable, str(SCRIPT_PATH), "name", "--manifest-path", str(manifest)],
check=False,
capture_output=True,
text=True,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 4
<location> `tests_python/test_read_manifest.py:87-94` </location>
<code_context>
result = subprocess.run(
[sys.executable, str(SCRIPT_PATH), "version"],
check=False,
capture_output=True,
text=True,
env=env,
cwd=self.temp_path,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 5
<location> `tests_python/test_read_manifest.py:101-106` </location>
<code_context>
result = subprocess.run(
[sys.executable, str(SCRIPT_PATH), "name", "--manifest-path", str(missing)],
check=False,
capture_output=True,
text=True,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 6
<location> `tests_python/test_read_manifest.py:113-118` </location>
<code_context>
result = subprocess.run(
[sys.executable, str(SCRIPT_PATH), "name", "--manifest-path", str(manifest)],
check=False,
capture_output=True,
text=True,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/scripts/read_manifest.py (1)
14-35: Add numpy-style docstrings to all public functions.The coding guidelines mandate numpy-style docstrings for all public interfaces.
parse_args,read_manifest,get_field, andmainare all missing structured documentation.Apply this diff to add a docstring to
parse_args:def parse_args() -> argparse.Namespace: + """ + Parse command-line arguments for the manifest reader. + + Returns + ------- + argparse.Namespace + Parsed arguments containing field name and optional manifest path. + """ parser = argparse.ArgumentParser(Similarly, add docstrings to
read_manifest(lines 38-42),get_field(lines 45-52), andmain(lines 55-67) following the numpy style guide. As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/scripts/read_manifest.py(1 hunks)tests_python/test_read_manifest.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep cyclomatic complexity ≤ 12
- Follow single responsibility and CQRS (command/query segregation)
- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename elements.
Files:
tests_python/test_read_manifest.py
🧬 Code graph analysis (1)
tests_python/test_read_manifest.py (1)
.github/workflows/scripts/read_manifest.py (2)
get_field(45-52)main(55-67)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (5)
.github/workflows/scripts/read_manifest.py (1)
63-63: LGTM – TOMLDecodeError now caught correctly.The addition of
tomllib.TOMLDecodeErrorensures invalid TOML syntax triggers the same error path as missing files or fields, which aligns with the PR objective to fail fast on malformed manifests.tests_python/test_read_manifest.py (4)
59-75: LGTM – CLI argument parsing covered.The test correctly validates that the script reads the manifest path from the
--manifest-pathargument and returns the expected field value with zero exit code.
77-97: LGTM – environment variable precedence verified.The test correctly validates that
CARGO_TOML_PATHis honoured when--manifest-pathis not provided, matching the documented behaviour inread_manifest.py.
99-109: LGTM – missing manifest error path validated.The test correctly verifies that a missing manifest file results in a non-zero exit code and an appropriate error message in stderr.
111-121: LGTM – invalid TOML error path validated.The test correctly verifies that malformed TOML triggers a non-zero exit code and error output, directly validating the new
tomllib.TOMLDecodeErrorhandling added in this PR.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
.github/actions/upload-release-assets/action.yml(1 hunks).github/workflows/release-dry-run.yml(1 hunks).github/workflows/release.yml(1 hunks)scripts/tests/test_upload_release_assets.py(1 hunks)scripts/upload_release_assets.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep cyclomatic complexity ≤ 12
- Follow single responsibility and CQRS (command/query segregation)
- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename elements.
Files:
scripts/tests/test_upload_release_assets.pyscripts/upload_release_assets.py
🧬 Code graph analysis (1)
scripts/tests/test_upload_release_assets.py (1)
scripts/upload_release_assets.py (2)
discover_assets(69-116)AssetError(37-38)
🪛 actionlint (1.7.7)
.github/workflows/release-dry-run.yml
34-34: property "bin_name" is not defined in object type {}
(expression)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (2)
.github/actions/upload-release-assets/action.yml (1)
1-31: LGTM!The action definition is clear and well-structured. The composite approach with environment variable passing (INPUT_* prefix) aligns correctly with the Cyclopts configuration in the target script (line 50 of
scripts/upload_release_assets.py). Strict shell options (set -euo pipefail) ensure early failure on errors. The absolute path reference via$GITHUB_WORKSPACEis appropriate for composite actions..github/workflows/release.yml (1)
302-306: LGTM!The step correctly delegates artefact upload to the new local action. The inputs align with the action's interface, and environment variables for the GitHub CLI remain properly configured.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests_python/test_read_manifest.py Comment on lines +99 to +109 def test_main_reports_missing_manifest(self) -> None:
missing = self.temp_path / "missing.toml"
result = subprocess.run(
[sys.executable, str(SCRIPT_PATH), "name", "--manifest-path", str(missing)],
check=False,
capture_output=True,
text=True,
)
self.assertNotEqual(result.returncode, 0)
self.assertIn("does not exist", result.stderr)
self.assertEqual(result.stdout, "")❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests_python/test_read_manifest.py Comment on lines +120 to +131 def test_main_reads_manifest_path_argument(self) -> None:
manifest = self._write_manifest(
"""
[package]
name = "netsuke"
version = "1.2.3"
"""
)
result = self._invoke_cli("name", "--manifest-path", str(manifest))
self.assertEqual(result.exit_code, 0)
self.assertEqual(result.stdout, "netsuke")
self.assertEqual(result.stderr, "")❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
tests_python/test_read_manifest.py (4)
46-51: Migrate to pytest fixture.This function should be replaced with a module-scoped pytest fixture as part of the unittest-to-pytest migration.
See the earlier comment on lines 1-18 for the complete migration strategy.
54-64: Migrate class setup to pytest fixtures.The
setUpClass,setUp, andtearDownmethods should be replaced with pytest fixtures.See the earlier comment on lines 1-18 for the complete migration strategy.
127-154: Parametrise the duplicate get_field tests.
test_get_field_returns_nameandtest_get_field_returns_versionshare identical structure. Replace them with a single parametrised test to eliminate duplication.After migrating to pytest (see lines 1-18), apply this refactor:
- def test_get_field_returns_name(self) -> None: - manifest = {"package": {"name": "netsuke", "version": "1.2.3"}} - self.assertEqual(self.module.get_field(manifest, "name"), "netsuke") - - def test_get_field_returns_version(self) -> None: - manifest = {"package": {"name": "netsuke", "version": "1.2.3"}} - self.assertEqual(self.module.get_field(manifest, "version"), "1.2.3") + @pytest.mark.parametrize( + "field,expected", + [ + ("name", "netsuke"), + ("version", "1.2.3"), + ], + ) + def test_get_field_returns_value(script_module, field, expected): + manifest = {"package": {"name": "netsuke", "version": "1.2.3"}} + assert script_module.get_field(manifest, field) == expectedAs per coding guidelines.
🤖 Prompt for AI agents
In tests_python/test_read_manifest.py around lines 127 to 133, replace the two duplicate tests test_get_field_returns_name and test_get_field_returns_version with a single pytest.mark.parametrize test named test_get_field_returns_value that iterates over ("name","netsuke") and ("version","1.2.3") and asserts get_field returns the expected value; use the script_module fixture as the first parameter; import pytest if not already imported; preserve the remaining tests test_get_field_raises_when_missing and test_get_field_rejects_non_string_values.
202-203: Remove after pytest migration.The
if __name__ == "__main__"block is unnecessary with pytest, which handles test discovery automatically.See the earlier comment on lines 1-18 for the complete migration strategy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests_python/test_read_manifest.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep cyclomatic complexity ≤ 12
- Follow single responsibility and CQRS (command/query segregation)
- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename elements.
Files:
tests_python/test_read_manifest.py
🧬 Code graph analysis (1)
tests_python/test_read_manifest.py (2)
scripts/tests/test_upload_release_assets.py (1)
module(18-26).github/workflows/scripts/read_manifest.py (2)
main(55-67)get_field(45-52)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (4)
tests_python/test_read_manifest.py (4)
21-22: LGTM!Correct use of pathlib for cross-platform path construction.
71-90: Expand the docstring to numpy style.This helper is complex and deserves structured documentation with Parameters, Returns, and Examples sections.
Apply this diff:
def _invoke_cli( self, *args: str, env: dict[str, str] | None = None, cwd: Path | None = None, ) -> CLIResult: - """Execute the CLI and capture its exit code and output streams.""" + """ + Execute the CLI in-process and capture its exit code and output streams. + + Parameters + ---------- + *args : str + Positional command-line arguments to pass to the script. + env : dict[str, str] | None, optional + Environment variables to patch into os.environ (default: None). + cwd : Path | None, optional + Working directory to change to during execution (default: None). + + Returns + ------- + CLIResult + A dataclass containing exit_code, stdout, and stderr. + + Examples + -------- + >>> result = self._invoke_cli("name", "--manifest-path", "Cargo.toml") + >>> result.exit_code + 0 + """As per coding guidelines.
Likely an incorrect or invalid review comment.
110-125: Add a numpy-style docstring.This helper lacks documentation. Provide a structured docstring with Parameters and Raises sections.
Apply this diff:
def _assert_successful_field_read( self, manifest_content: str, field: str, expected_value: str, *, cli_args: tuple[str, ...] | None = None, env: dict[str, str] | None = None, cwd: Path | None = None, ) -> None: + """ + Assert that the CLI successfully reads a field from a manifest. + + Parameters + ---------- + manifest_content : str + The TOML content to write to Cargo.toml. + field : str + The field name to read from the manifest. + expected_value : str + The expected value of the field. + cli_args : tuple[str, ...] | None, optional + Custom CLI arguments; defaults to (field, "--manifest-path", manifest_path) (default: None). + env : dict[str, str] | None, optional + Environment variables to patch (default: None). + cwd : Path | None, optional + Working directory to change to (default: None). + + Raises + ------ + AssertionError + If the CLI does not succeed or returns unexpected output. + """ manifest = self._write_manifest(manifest_content)As per coding guidelines.
Likely an incorrect or invalid review comment.
92-108: Add a numpy-style docstring.This helper method lacks documentation. Private methods should have at least a single-line docstring, but given its complexity, a structured docstring is appropriate.
Apply this diff:
def _assert_manifest_error( self, manifest_path: Path, expected_stderr_fragment: str | None = None, ) -> None: + """ + Assert that the CLI fails when given an invalid manifest path. + + Parameters + ---------- + manifest_path : Path + The path to the manifest file to test. + expected_stderr_fragment : str | None, optional + A substring expected in stderr; if None, only checks stderr is non-empty (default: None). + + Raises + ------ + AssertionError + If the CLI does not fail as expected. + """ result = subprocess.run(As per coding guidelines.
Likely an incorrect or invalid review comment.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests_python/test_read_manifest.py (4)
33-41: Expand the docstring to numpy style with Parameters, Yields, and Examples.Public functions must have structured numpy-style docstrings. Add Parameters, Yields, and Examples sections to document this context manager properly.
Apply this diff:
@contextmanager def change_directory(path: Path) -> typ.Iterator[None]: - """Temporarily change the working directory for the current process.""" + """ + Temporarily change the working directory for the current process. + + Parameters + ---------- + path : Path + The target directory. + + Yields + ------ + None + Control returns to the caller with the directory changed. + + Examples + -------- + >>> with change_directory(Path("/tmp")): + ... print(Path.cwd()) + /tmp + """ original = Path.cwd() os.chdir(path) try: yield finally: os.chdir(original)As per coding guidelines.
24-30: Convert the docstring to numpy style with an Attributes section.The docstring uses Sphinx notation (
:func:), which violates the coding guidelines mandating numpy style for all docstrings. Add a proper Attributes section documenting each field.Apply this diff:
@dataclasses.dataclass(slots=True) class CLIResult: - """Result container returned by :func:`ReadManifestTests._invoke_cli`.""" + """ + Result container returned by ReadManifestTests._invoke_cli. + + Attributes + ---------- + exit_code : int + The exit code returned by the CLI invocation. + stdout : str + The captured standard output. + stderr : str + The captured standard error. + """ exit_code: int stdout: str stderr: strAs per coding guidelines.
44-52: Inline this function into the pytest fixture or mark it private.The
load_script_modulefunction is only called once from theread_manifest_modulefixture (line 141). Either inline the logic directly into the fixture to reduce indirection, or prefix the function with_to indicate it is private.Option 1: Inline into the fixture (preferred):
@pytest.fixture(scope="module") def read_manifest_module() -> types.ModuleType: """Load the read_manifest script once for all tests.""" - return load_script_module() + spec = importlib.util.spec_from_file_location("read_manifest", SCRIPT_PATH) + module = importlib.util.module_from_spec(spec) # type: ignore[arg-type] + assert spec is not None + assert spec.loader is not None + spec.loader.exec_module(module) # type: ignore[assignment] + assert isinstance(module, types.ModuleType) + return module - - -def load_script_module() -> types.ModuleType: - """Import the read_manifest script as a module for reuse in tests.""" - spec = importlib.util.spec_from_file_location("read_manifest", SCRIPT_PATH) - module = importlib.util.module_from_spec(spec) # type: ignore[arg-type] - assert spec is not None - assert spec.loader is not None - spec.loader.exec_module(module) # type: ignore[assignment] - assert isinstance(module, types.ModuleType) - return moduleOption 2: Mark as private:
-def load_script_module() -> types.ModuleType: +def _load_script_module() -> types.ModuleType: """Import the read_manifest script as a module for reuse in tests.""" spec = importlib.util.spec_from_file_location("read_manifest", SCRIPT_PATH) module = importlib.util.module_from_spec(spec) # type: ignore[arg-type] assert spec is not None assert spec.loader is not None spec.loader.exec_module(module) # type: ignore[assignment] assert isinstance(module, types.ModuleType) return module @pytest.fixture(scope="module") def read_manifest_module() -> types.ModuleType: """Load the read_manifest script once for all tests.""" - return load_script_module() + return _load_script_module()
1-1: Expand the module docstring to numpy style with full sections.The current single-line docstring violates the coding guidelines, which mandate that every module begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls. Expand this to numpy style with Summary, Purpose, Utility, Usage, and Examples sections.
Apply this diff:
-"""Tests for the read_manifest helper script.""" +""" +Tests for the read_manifest helper script. + +This module validates the behaviour of `.github/workflows/scripts/read_manifest.py`, +which extracts metadata fields from Cargo.toml manifests. It exercises both the +`get_field` function (for programmatic field extraction) and the CLI entry point +(for error handling of missing files, invalid TOML, and structural issues). + +Utility +------- +Ensures robust error handling when manifests are absent, malformed, or missing +required sections, and verifies correct field extraction for valid manifests. + +Usage +----- +Run the test suite with pytest: + + python -m pytest tests_python/test_read_manifest.py + +Examples +-------- +Run a single test: + + python -m pytest tests_python/test_read_manifest.py::test_main_reports_invalid_toml + +"""As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests_python/test_read_manifest.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep cyclomatic complexity ≤ 12
- Follow single responsibility and CQRS (command/query segregation)
- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename elements.
Files:
tests_python/test_read_manifest.py
🧬 Code graph analysis (1)
tests_python/test_read_manifest.py (2)
scripts/tests/test_upload_release_assets.py (1)
module(18-26).github/workflows/scripts/read_manifest.py (2)
main(55-67)get_field(45-52)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (1)
tests_python/test_read_manifest.py (1)
248-258: LGTM! Inconsistency resolved.The test now uses
_assert_manifest_error(external subprocess invocation), which aligns with the other manifest error tests (test_main_reports_missing_manifestandtest_main_reports_invalid_toml). This addresses the inconsistency flagged in earlier review comments.Based on past review comments.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
tests_python/test_read_manifest.py (2)
42-50: Expand the docstring to numpy style.The function lacks structured documentation. Add Parameters, Yields and Examples sections.
Apply this diff:
@contextmanager def change_directory(path: Path) -> typ.Iterator[None]: - """Temporarily change the working directory for the current process.""" + """ + Temporarily change the working directory for the current process. + + Parameters + ---------- + path : Path + The target directory. + + Yields + ------ + None + Control returns to the caller with the directory changed; the original + directory is restored upon exit. + + Examples + -------- + >>> with change_directory(Path("/tmp")): + ... print(Path.cwd()) + /tmp + """As per coding guidelines.
24-30: Replace Sphinx notation with numpy-style docstring.The docstring uses Sphinx
:func:notation. Replace it with a numpy-style docstring including an Attributes section.Apply this diff:
@dataclasses.dataclass(slots=True) class CLIResult: - """Result container returned by :func:`ReadManifestTests._invoke_cli`.""" + """ + Result container returned by ReadManifestTests._invoke_cli. + + Attributes + ---------- + exit_code : int + The exit code returned by the CLI invocation. + stdout : str + The captured standard output. + stderr : str + The captured standard error. + """As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/release.yml(2 hunks)scripts/tests/test_upload_release_assets.py(1 hunks)scripts/upload_release_assets.py(1 hunks)tests_python/test_read_manifest.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep cyclomatic complexity ≤ 12
- Follow single responsibility and CQRS (command/query segregation)
- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename elements.
Files:
scripts/upload_release_assets.pytests_python/test_read_manifest.pyscripts/tests/test_upload_release_assets.py
🧬 Code graph analysis (2)
tests_python/test_read_manifest.py (2)
scripts/tests/test_upload_release_assets.py (1)
module(18-26).github/workflows/scripts/read_manifest.py (2)
main(55-67)get_field(45-52)
scripts/tests/test_upload_release_assets.py (1)
scripts/upload_release_assets.py (2)
discover_assets(94-131)AssetError(39-40)
🔍 Remote MCP Ref
Below are targeted findings relevant to the PR review:
-
Tomllib Availability
• Python’s built-in tomllib module (and its TOMLDecodeError) was added in 3.11. If this project still supports Python ≤3.10, catching tomllib.TOMLDecodeError will fail at import time. Ensure the CI matrix or pyproject.toml declares “python_requires ≥3.11” or guard the import accordingly.
Source: Python 3.11 release notes (standard library) [https://docs.python.org/3.11/library/tomllib.html] -
Existing Version Constraint
• The project’s pyproject.toml sets “python_requires = '>=3.8'” (earlier commit).
• No back-ported tomllib shim is vendored.
Risk: On 3.8–3.10 runners,import tomllibwill raise ModuleNotFoundError before the CLI can catch TOMLDecodeError. -
Recommended Fix
• Either update python_requires in pyproject.toml to “>=3.11” or wrapimport tomllibin a try/except to fallback toimport tomliif installed, or catch ModuleNotFoundError and print an actionable error.
• Add a test for a missing tomllib import environment to ensure the script errors gracefully on older Pythons. -
CI Validation
• Confirm that GitHub Actions workflows (release.yml, release-dry-run.yml) specify a Python-3.11 runner.
• If not, update.github/workflows/…to include “runs-on: ubuntu-latest”, “strategy: matrix: python-version: [3.11]” or adjust accordingly.
These points ensure the new TOMLDecodeError catch works reliably across supported Python versions.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (18)
scripts/upload_release_assets.py (12)
1-36: LGTM!The module docstring meets the coding guidelines (purpose, utility, usage with examples). Imports are correctly organised and typed. The Python version requirement (≥3.13) aligns with the shebang and dependencies.
38-49: LGTM!The exception and dataclass definitions are clear and well-typed. The frozen dataclass ensures immutability for release assets.
55-61: LGTM!The candidate filter logic is correct and uses pathlib for safe path manipulation. Cyclomatic complexity is low.
64-68: LGTM!The asset naming logic correctly handles platform-specific packages and prefixes others with the parent directory name. Pathlib usage is appropriate.
71-74: LGTM!The candidate path iterator correctly uses pathlib's rglob and sorts results for deterministic discovery. The filter logic is sound.
77-81: LGTM!The size validation correctly rejects empty files and uses pathlib for stat access. The error message is clear.
84-91: LGTM!The collision detection correctly prevents duplicate asset names and provides a clear error message identifying both conflicting paths.
94-131: LGTM!The public function has a complete NumPy-style docstring as required. The discovery logic is clear, handles errors appropriately, and maintains deterministic ordering through sorted paths. Type hints are comprehensive.
134-140: LGTM!The summary renderer correctly formats the asset list for dry-run output with clear metadata (name, size, path).
143-180: LGTM!The upload function has the required NumPy-style docstring (past review comment addressed). The lazy initialisation of gh_cmd optimises dry-run performance. Type hints are comprehensive, including the BoundCommand | None annotation (past review comment addressed).
183-226: LGTM!The main entry point has the required NumPy-style docstring (past review comment addressed). Error handling correctly catches specific plumbum exceptions (past review comment addressed) and provides clear exit codes. The dry-run summary is printed before upload, improving user experience.
229-248: LGTM!The CLI binding correctly uses cyclopts annotations to enforce required parameters and delegates to the testable main function. The script guard properly raises SystemExit with the app's return code.
scripts/tests/test_upload_release_assets.py (6)
1-14: LGTM!The module docstring and imports are correctly structured. Constants use pathlib for safe path manipulation.
17-26: LGTM!The fixture correctly imports the script in-process using importlib. The type: ignore suppressions now have the required FIXME comments (past review comment addressed), and the assertion includes a clear failure message (past review comment addressed).
34-53: LGTM!The test has the required single-line docstring (past review comment addressed). It correctly exercises asset discovery with multiple platforms and verifies the expected ordering and naming conventions.
56-65: LGTM!The test has the required single-line docstring (past review comment addressed). It correctly validates that asset name collisions are detected and reported with a clear error message.
68-76: LGTM!The test has the required single-line docstring (past review comment addressed). It correctly validates that empty files are rejected with an appropriate error message.
79-106: LGTM!The test has the required single-line docstring (past review comment addressed). It correctly exercises the CLI in dry-run mode and validates both the summary output and the planned command formatting.
Expand the read_manifest test module docstrings, describe helper\nconfiguration fields, and document the upload asset test helper. Update\nthe release workflow output description to clarify the bin name source.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/scripts/read_manifest.py (3)
1-2: Expand the module docstring to numpy style with usage examples.The current docstring is too brief. Add sections for Summary, Purpose, Usage, and Examples demonstrating typical invocations of the CLI and API functions.
Apply this diff:
-"""Utility helpers for extracting fields from Cargo.toml.""" +""" +Utility helpers for extracting fields from Cargo.toml. + +Summary +------- +Parse and extract package metadata fields from Cargo manifest files. + +Purpose +------- +Provide both a CLI tool and programmatic API for reading ``name`` and +``version`` fields from Cargo.toml manifests, with robust error handling +for missing files, invalid TOML, and unexpected structure. + +Usage +----- +CLI invocation:: + + python read_manifest.py name --manifest-path /path/to/Cargo.toml + python read_manifest.py version + +Programmatic usage:: + + from pathlib import Path + manifest = read_manifest(Path("Cargo.toml")) + name = get_field(manifest, "name") + +Examples +-------- +Extract the package name from a manifest:: + + $ python read_manifest.py name --manifest-path Cargo.toml + netsuke + +Use the CARGO_TOML_PATH environment variable:: + + $ export CARGO_TOML_PATH=/path/to/Cargo.toml + $ python read_manifest.py version + 1.2.3 +"""As per coding guidelines.
21-36: Expand the docstring to numpy style for this public function.Public functions must have structured numpy-style docstrings with Returns and Examples sections.
Apply this diff:
def parse_args() -> argparse.Namespace: - """Return the parsed CLI arguments for manifest field extraction.""" + """ + Return the parsed CLI arguments for manifest field extraction. + + Returns + ------- + argparse.Namespace + Parsed arguments containing ``field`` (str) and optional + ``manifest_path`` (str or None). + + Examples + -------- + >>> args = parse_args() # With sys.argv = ["script.py", "name"] + >>> args.field + 'name' + """ parser = argparse.ArgumentParser(description=PARSER_DESCRIPTION)As per coding guidelines.
61-74: Add structured numpy-style docstring for this public entry point.The main function is a public interface and requires full structured documentation per the coding guidelines.
Apply this diff:
def main() -> int: - """Entry point for the manifest reader CLI.""" + """ + Entry point for the manifest reader CLI. + + Returns + ------- + int + Exit code: 0 for success, 1 for errors (missing file, invalid + TOML, or missing fields). + + Examples + -------- + Typical CLI invocation:: + + $ python read_manifest.py name --manifest-path Cargo.toml + netsuke + """ args = parse_args()As per coding guidelines.
♻️ Duplicate comments (2)
scripts/tests/test_upload_release_assets.py (1)
100-115: Restore required FIXME annotation on noqa suppression.
Add the mandatoryFIXME:note to the# noqa: S603suppression so the comment satisfies the documented format.- result = subprocess.run( # noqa: S603 # Security: CLI invocation uses trusted arguments in tests. + result = subprocess.run( # noqa: S603 # FIXME: subprocess required for CLI integration test with trusted argumentsAs per coding guidelines.
tests_python/test_read_manifest.py (1)
79-85: Replace Sphinx notation with numpy-style Attributes section.The docstring uses
:func:notation but should follow numpy style with an Attributes section documenting each field.Apply this diff:
@dataclasses.dataclass(slots=True) class CLIResult: - """Result container returned by :func:`ReadManifestTests._invoke_cli`.""" + """ + Result container returned by ReadManifestTests._invoke_cli. + + Attributes + ---------- + exit_code : int + The exit code returned by the CLI invocation. + stdout : str + The captured standard output. + stderr : str + The captured standard error. + """As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/scripts/read_manifest.py(2 hunks)scripts/tests/test_upload_release_assets.py(1 hunks)scripts/upload_release_assets.py(1 hunks)tests_python/test_read_manifest.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep cyclomatic complexity ≤ 12
- Follow single responsibility and CQRS (command/query segregation)
- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename elements.
Files:
tests_python/test_read_manifest.pyscripts/upload_release_assets.pyscripts/tests/test_upload_release_assets.py
🧬 Code graph analysis (3)
tests_python/test_read_manifest.py (2)
scripts/tests/test_upload_release_assets.py (1)
module(24-36).github/workflows/scripts/read_manifest.py (2)
main(61-74)get_field(48-58)
scripts/upload_release_assets.py (1)
.github/workflows/scripts/read_manifest.py (1)
main(61-74)
scripts/tests/test_upload_release_assets.py (1)
scripts/upload_release_assets.py (2)
discover_assets(100-138)AssetError(43-44)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (ubuntu-latest, stable)
- GitHub Check: Sourcery review
🔇 Additional comments (20)
.github/workflows/scripts/read_manifest.py (2)
13-18: LGTM!The PARSER_DESCRIPTION constant improves maintainability by extracting the description into a named constant.
70-70: LGTM!Adding
tomllib.TOMLDecodeErrorto the exception handler correctly addresses invalid TOML content, ensuring the CLI fails fast with a descriptive error message.tests_python/test_read_manifest.py (18)
1-56: LGTM!The module docstring is comprehensive and follows numpy style with clear sections for Summary, Purpose, Usage, and Examples.
58-76: LGTM!Imports and path constants are well-organised and use pathlib as required by the coding guidelines.
88-105: LGTM!The CLIInvocationConfig dataclass has excellent numpy-style documentation with a clear Attributes section.
119-145: LGTM!The function has excellent error handling and a well-structured numpy-style docstring with Returns and Examples sections.
148-165: LGTM!The class docstring includes a helpful Methods section documenting the available helpers.
170-174: LGTM!The helper method has a clear single-line docstring appropriate for a private method.
176-200: LGTM!The in-process CLI invocation helper correctly uses ExitStack to manage multiple context managers and capture output streams.
202-225: LGTM!The subprocess invocation is appropriately annotated with a noqa comment explaining that inputs are trusted in the test context.
227-242: LGTM!The helper method uses a configuration object to reduce parameter count whilst maintaining flexibility, following good design practices.
245-257: LGTM!The fixtures are well-designed with appropriate scopes and clear docstrings.
260-274: LGTM!The parametrised test efficiently covers multiple field extractions with a single test function, following pytest best practices.
277-281: LGTM!The test correctly verifies that missing fields raise KeyError.
284-301: LGTM!The test comprehensively covers rejection of non-string manifest entries (integers, lists, dictionaries), ensuring robust type validation.
304-316: LGTM!The test uses the
_assert_successful_field_readhelper effectively to verify manifest path argument handling.
319-338: LGTM!The test correctly verifies that the CARGO_TOML_PATH environment variable takes precedence over the default location.
341-346: LGTM!The test uses the
_assert_manifest_errorhelper to verify error handling for missing manifest files.
349-352: LGTM!The test verifies error handling for invalid TOML content, directly supporting the PR objective of catching TOMLDecodeError.
355-365: LGTM!The test verifies descriptive error reporting when the manifest has valid TOML but missing required sections, ensuring robust validation.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/scripts/read_manifest.py (2)
21-36: Add structured docstring forparse_args.
Replace the single-line docstring with a NumPy-style docstring that documents the return value and shows a basic usage example to satisfy the documentation standard.def parse_args() -> argparse.Namespace: - """Return the parsed CLI arguments for manifest field extraction.""" + """ + Parse CLI arguments for manifest field extraction. + + Returns + ------- + argparse.Namespace + The parsed CLI arguments containing the requested field and manifest path. + + Examples + -------- + >>> import sys + >>> sys.argv = ["read_manifest.py", "name"] + >>> parse_args().field + 'name' + """As per coding guidelines.
113-126: Documentmainwith full NumPy-style sections.
Expand the docstring so it explains the return code semantics and demonstrates a valid invocation, aligning with the documentation requirement for public interfaces.def main() -> int: - """Entry point for the manifest reader CLI.""" + """ + Execute the manifest reader CLI. + + Returns + ------- + int + Zero when the requested field is printed successfully; one when an error is reported. + + Examples + -------- + >>> import sys + >>> from pathlib import Path + >>> manifest = Path("Cargo.toml") + >>> _ = manifest.write_text("[package]\nname = 'netsuke'\nversion = '1.2.3'\n", encoding="utf-8") + >>> sys.argv = ["read_manifest.py", "name", "--manifest-path", str(manifest)] + >>> main() + 0 + """As per coding guidelines.
♻️ Duplicate comments (1)
tests_python/test_read_manifest.py (1)
80-86: Replace the short docstring onCLIResult.
Switch to a NumPy-style docstring that removes the Sphinx role and documents each attribute so the class meets the documentation requirement.@dataclasses.dataclass(slots=True) class CLIResult: - """Result container returned by :func:`ReadManifestTests._invoke_cli`.""" + """ + Result container returned by ReadManifestTests._invoke_cli. + + Attributes + ---------- + exit_code : int + Exit status from the CLI invocation. + stdout : str + Captured standard output. + stderr : str + Captured standard error. + """As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/scripts/read_manifest.py(2 hunks)tests_python/test_read_manifest.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep cyclomatic complexity ≤ 12
- Follow single responsibility and CQRS (command/query segregation)
- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename elements.
Files:
tests_python/test_read_manifest.py
🧬 Code graph analysis (1)
tests_python/test_read_manifest.py (2)
scripts/tests/test_upload_release_assets.py (1)
module(24-36).github/workflows/scripts/read_manifest.py (2)
main(113-126)get_field(75-110)
🔍 Remote MCP Ref
Additional Context for PR #179 Review
-
read_manifest.pyCLI Description Constant
The newPARSER_DESCRIPTIONconstant defines the CLI help description, replacing inline strings:- Originally passed inline to
ArgumentParser(description=…), now centralized inPARSER_DESCRIPTIONfor consistency and reuse.
- Originally passed inline to
-
TOML Decode Error Handling
The updatedread_manifestandmainfunctions now explicitly catchtomllib.TOMLDecodeErrorin addition toKeyErrorandFileNotFoundError, printing an error message and exiting with code 1. This ensures invalid TOML is handled gracefully. -
Unit Tests for Error Paths
- Tests for missing manifest path and invalid TOML now exist (
test_main_reports_missing_manifest,test_main_reports_invalid_toml). - Proposed helper
_assert_manifest_errorshould consolidate duplicate subprocess invocation and assertions across these tests.
- Tests for missing manifest path and invalid TOML now exist (
-
Unit Tests for Successful Reads
- Tests
test_main_reads_manifest_path_argumentand related validate return code 0 and correct stdout. - Proposed helper
_assert_successful_field_readcould replace duplication in manifest creation, CLI invocation, and assertions.
- Tests
-
Test Duplication and Refactor Opportunities
- Two pairs of tests share identical setup/invocation/assert patterns.
- Refactor with helper methods in
ReadManifestTeststo reduce duplication while preserving existing_write_manifestand_invoke_climethods.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (ubuntu-latest, stable)
- GitHub Check: Sourcery review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/release.yml(3 hunks)ruff.toml(2 hunks)tests_python/test_read_manifest.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep cyclomatic complexity ≤ 12
- Follow single responsibility and CQRS (command/query segregation)
- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename elements.
Files:
tests_python/test_read_manifest.py
🧬 Code graph analysis (1)
tests_python/test_read_manifest.py (2)
scripts/tests/test_upload_release_assets.py (1)
module(24-36).github/workflows/scripts/read_manifest.py (2)
main(113-126)get_field(75-110)
🔍 Remote MCP Deepwiki
Summary of additional relevant context for reviewing PR #179
-
Repository-level docs and code organization show a strong test-first, strict-linting culture; CI enforces formatting, lint, and tests (make check-fmt / make lint / make test). Useful to keep tests DRY and consistent with existing test patterns and CI expectations.
-
Manifest parsing and template work in the project is centralized in a manifest/AST pipeline (minijinja → serde_yml → AST → IR). read_manifest.py interacts with Cargo.toml files only for CI/workflow metadata; catching tomllib.TOMLDecodeError in the CLI aligns with existing parsing/error patterns (fail fast, contextual messages).
-
The PR adds unit tests that exercise read_manifest.py via both in-process module import and subprocess CLI invocation; the PR discussion identifies duplicated subprocess invocation/assertion patterns across tests and proposes two test helpers in ReadManifestTests to remove duplication:
- _assert_manifest_error(manifest_path, expected_stderr_fragment=None): run subprocess, assert non-zero exit, stderr content, and empty stdout.
- _assert_successful_field_read(manifest_content, field, expected_value, *, cli_args=None, env=None, cwd=None): write manifest, invoke CLI (or use env override), assert exit_code==0, stdout==expected_value, stderr=="".
Implementing these helpers will keep new tests consistent with the project's fixture/helper patterns and reduce duplication flagged by reviewer. [PR description / comments in provided context]
-
The PR also introduces a new local GitHub Action and script for release uploads (.github/actions/upload-release-assets and scripts/upload_release_assets.py) plus tests for that script. These add new public script entities (AssetError, ReleaseAsset, discover_assets, upload_assets, main, cli). Review should verify:
- discover_assets enforces non-zero file sizes and asset-name uniqueness (raises AssetError on duplicates/zero-byte).
- Dry-run output formatting in _render_summary matches workflow validation usage (release-dry-run.yml calls the action with dry-run=true).
- Tests cover discovery ordering, duplicate rejection, empty-file rejection, and CLI dry-run summary.
Key review implications / action items
- Accept PR’s tomllib.TOMLDecodeError handling change — it matches repository error-handling practices (fail-fast, contextual message).
- For tests_python/test_read_manifest.py: apply the suggested helper refactors to remove duplicated subprocess invocation and assertions (use the two helpers described) so tests are concise and aligned with repo test helper patterns.
- For scripts/upload_release_assets.py and its tests: verify discover_assets edge cases (duplicate names, zero-byte files) and that dry-run integration is compatible with the workflow changes in release-dry-run.yml and release.yml which now call the local action. Ensure new public symbols are documented and that tests run under CI lint/format rules.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (2)
ruff.toml (2)
41-41: Retain the task tag configuration.Retain this list to keep Ruff’s TD checks focused on TODO and FIXME markers, which matches the project’s linting expectations.
65-65: Keep the docstring convention note.Keep this reminder so contributors follow the enforced NumPy docstring style without ambiguity.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Blocking issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests_python/test_read_manifest.py:238-249` </location>
<code_context>
result = subprocess.run( # noqa: S603 # TODO(release-ci): FIXME: Security false positive; executed with trusted inputs in tests. https://github.com/leynos/netsuke/pull/179#discussion_r2404108802
[
sys.executable,
str(SCRIPT_PATH),
"name",
"--manifest-path",
str(manifest_path),
],
check=False,
capture_output=True,
text=True,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 2
<location> `scripts/upload_release_assets.py:90-91` </location>
<code_context>
def _register_asset(asset_name: str, path: Path, seen: dict[str, Path]) -> None:
previous = seen.get(asset_name)
if previous:
message = (
"Asset name collision: "
f"{asset_name} would upload both {previous} and {path}"
)
raise AssetError(message)
seen[asset_name] = path
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
```suggestion
if previous := seen.get(asset_name):
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@sourcery-ai review |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/scripts/read_manifest.py(3 hunks)scripts/upload_release_assets.py(1 hunks)tests_python/test_read_manifest.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep cyclomatic complexity ≤ 12
- Follow single responsibility and CQRS (command/query segregation)
- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename elements.
Files:
scripts/upload_release_assets.pytests_python/test_read_manifest.py
🧬 Code graph analysis (2)
.github/workflows/scripts/read_manifest.py (1)
scripts/upload_release_assets.py (1)
main(188-230)
tests_python/test_read_manifest.py (2)
scripts/tests/test_upload_release_assets.py (1)
module(24-36).github/workflows/scripts/read_manifest.py (2)
main(165-193)get_field(127-162)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6015 🏁 Script executed: Length of output: 2012 @leynos: Yes, both issues have been resolved: 1. ✅ Dry-run boolean handlingThe string
def _coerce_bool(value: object) -> bool:
"""Return ``value`` as a strict boolean."""
if isinstance(value, bool):
return value
if not isinstance(value, str):
message = f"Cannot interpret {value!r} as a boolean"
raise TypeError(message)
normalised = value.strip().lower()
if normalised in {"", "false", "0", "no", "off"}:
return False
if normalised in {"true", "1", "yes", "on"}:
return True
# ...2. ✅ Metadata job outputsThe metadata:
runs-on: ubuntu-latest
outputs:
version: ${{ steps.ensure_version.outputs['crate-version'] }}
bin_name: ${{ steps.bin_name.outputs.value }}
should_publish: ${{ steps.publish.outputs.value }}Both |
leynos
deleted the
codex/address-code-review-comments-for-read_manifest.py
branch
1 participant
Summary
tomllib.TOMLDecodeErrorin the manifest reader CLI so invalid TOML fails fastTesting
https://chatgpt.com/codex/tasks/task_e_68de5d76681c83229e66aaeefe15c1a8
Summary by Sourcery
Improve the manifest reader CLI to handle invalid TOML, refactor release workflows to use a new Python upload-release-assets action, update lint settings, and add comprehensive tests for both tools
New Features:
Enhancements:
CI:
Tests: